Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update doc with Attribute::unescaped_values #1

Closed
wants to merge 1 commit into from

Conversation

tafia
Copy link

@tafia tafia commented Mar 8, 2018

You're comment about the xml snippet was wrong (& instead of &).
I have also added an example of how you should get the unescaped_values for people (wrongly) relying on BytesStart::unescaped today.

@MarkDDR
Copy link
Owner

MarkDDR commented Mar 8, 2018

You're completely right about the &, such an easy thing to overlook, and I definitely should have added tests for the attributes as well.

I would also add a test for the attribute keys, since people generally care about both the key and the value. There doesn't seem to be any unescape methods for keys but having escaped things in the key might just not be allowed for xml, I'm not an expert in that regard. Regardless having an assert_eq! with some key value would be nice.

Finally, I think the proper way to deal with coercing the Cow/BytesStart/whatever to a &[u8] (or any other deref impl) is to use .as_ref(), instead of &*var. Obviously both work, and I feel this is mostly just a style note than anything else, though at the same time .as_ref() is a lot easier to try than desperately adding &* to make coercions work (especially for people who don't know what's going on).

@MarkDDR
Copy link
Owner

MarkDDR commented Mar 8, 2018

Tangent: Actually the whole & thing reminds me of a bit of how there are a bunch of invariants that the user needs to enforce. Like when creating the BytesStart, the bytestring must be escaped, and the same goes for attributes unless you use a &str instead of a bytestring, in which case it will escape for you. In theory we could do checks everytime and make unchecked bytestrings be some unsafe function, though that feels like a bad ergonomic and performance hit for what probably doesn't matter most of the time. Probably. This deserves its own separate issue though

@tafia tafia closed this Sep 5, 2019
@tafia tafia deleted the pr119_2 branch September 5, 2019 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants